Skill to compare performance of a branch or PR with main - #22725

.agents/skills/perf-compare-cudf/scripts/compare.py (2)

120-122: ⚡ Quick win

Consider adding error handling for malformed CSV files.

If a CSV file is malformed, csv.DictReader may raise exceptions that would crash the script with an unclear error message. Adding a try/except block would improve user experience.

🛡️ Proposed fix to handle CSV errors gracefully

 def read_rows(path: Path) -> list[dict]:
-    with open(path) as f:
-        return list(csv.DictReader(f))
+    try:
+        with open(path) as f:
+            return list(csv.DictReader(f))
+    except (OSError, csv.Error) as e:
+        raise SystemExit(f"Failed to read {path}: {e}")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.agents/skills/perf-compare-cudf/scripts/compare.py around lines 120 - 122,
The read_rows function uses csv.DictReader without handling malformed CSVs; wrap
the file read and csv.DictReader invocation in a try/except inside read_rows to
catch csv.Error (and optionally UnicodeDecodeError/IOError), log or raise a
clearer, contextual error mentioning the path, and return an empty list or
rethrow a custom exception depending on caller expectations; update references
to read_rows to handle the new return/error behavior if needed.

---

305-328: ⚡ Quick win

Consider validating input directories in argument parsing.

The script doesn't validate that --pr and --main point to existing directories. If they don't exist or are files, the error messages later will be unclear (e.g., "no CSVs found" when the directory doesn't exist).

✅ Proposed fix to validate directories early

Add a custom type for directory validation:

+def existing_dir(path_str: str) -> Path:
+    path = Path(path_str)
+    if not path.is_dir():
+        raise argparse.ArgumentTypeError(f"{path} is not a directory")
+    return path
+
 def parse_args() -> argparse.Namespace:
     parser = argparse.ArgumentParser(
         description=__doc__,
         formatter_class=argparse.RawDescriptionHelpFormatter,
     )
     parser.add_argument(
-        "--pr", required=True, type=Path, help="dir with PR-branch CSVs"
+        "--pr", required=True, type=existing_dir, help="dir with PR-branch CSVs"
     )
     parser.add_argument(
-        "--main", required=True, type=Path, help="dir with main-branch CSVs"
+        "--main", required=True, type=existing_dir, help="dir with main-branch CSVs"
     )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.agents/skills/perf-compare-cudf/scripts/compare.py around lines 305 - 328,
The parse_args function currently accepts --pr and --main as Path but doesn't
validate they exist or are directories; add early validation so users get clear
errors: either implement a custom argparse type/validator (used for the --pr and
--main arguments) that checks path.exists() and path.is_dir() and raises
argparse.ArgumentTypeError on failure, or after parser.parse_args() check
args.pr and args.main and raise argparse.ArgumentTypeError (or call
parser.error) if they are missing or not directories; update parse_args to
reference the new validator so bad inputs fail fast and with a clear message.